feat: improve session ergonomics via server-provided session hints#260
feat: improve session ergonomics via server-provided session hints#260brendanjryan wants to merge 6 commits intomainfrom
Conversation
commit: |
decofe
left a comment
There was a problem hiding this comment.
🛡️ Cyclops Security Review — Consolidated Findings
Commit: 727181b
Mode: normal · 3 workers (gemini-3.1-pro-preview, amp/deep, gpt-5.4)
Verified Findings
After multi-engine audit and independent verification, 2 findings were confirmed:
🚨 Finding 1: Malicious Server Can Drain Client Deposit via Forged Session Hints — Critical
Affected code: src/tempo/client/ChannelOps.ts:L124-L127, src/tempo/client/Session.ts:L291-L293, src/client/internal/Fetch.ts
Description: The monotonic session hint reconciliation path blindly trusts server-provided acceptedCumulative, spent, and requiredCumulative hints, using them to overwrite entry.cumulativeAmount (the sum of authorized payments). A malicious server can forge these hints to trick the client into signing a voucher for the entire channel deposit.
Attack path:
- Client opens a channel with deposit of 1000 tokens, requests a $5 resource.
- Malicious server responds with 402 challenge containing forged hints:
requiredCumulative = 1000(or returnsPayment-Receiptwithspent = 1000). reconcileChannelEntryandautoManageCredentialblindly adopt this value, updating localentry.cumulativeAmountto 1000.autoManageCredentialsigns a voucher authorizing 1000 tokens and sends it to the server.- Server settles the voucher on-chain, stealing the full deposit.
Root cause: reconcileChannelEntry() raises local channel state from server-provided values without bounding, and autoManageCredential() uses max(entry.cumulativeAmount + amount, requiredCumulative) before signing.
Recommended fix: The client must never increase entry.cumulativeAmount based on untrusted server hints. Remove the entry.cumulativeAmount overrides in reconcileChannelEntry, and remove the requiredCumulative bump in autoManageCredential. Server hints may warn of desynchronization but must not inflate cryptographic authorization.
🚨 Finding 2: Stale channel aliases let old receipts poison a fresh channel — High
Affected code: src/tempo/client/Session.ts:L121-L125 (rememberChannel), src/tempo/client/Session.ts:L199-L206 (reconcileReceipt), src/tempo/client/ChannelOps.ts:L115-L161
Description: rememberChannel() adds channelId → key entries for every recovered/hinted channel but never removes the old mapping when the same (payee, currency, escrow) key is rebound to a new channel. A legitimate old receipt for channel A can ratchet the signable state of channel B via the stale alias.
Attack path:
- Client binds key
payee:currency:escrowto channel A. - Server rotates session to channel B for the same key —
channels[key]is overwritten to B, butchannelIdToKey[A] = keyis not removed. - A replayed
Payment-Receiptfor channel A hitsreconcileReceipt(). - Lookup resolves:
channelIdToKey[A] → key → channels[key] → channel B(without verifyingentry.channelId === receipt.channelId). reconcileChannelReceipt()raises B'sacceptedCumulative,spent, andcumulativeAmountfrom A's receipt.- Next auto-managed request signs an inflated voucher on B.
Recommended fix: Remove stale alias when rebinding a key to a new channel. Add a guard in reconcileReceipt to reject any receipt whose channelId does not match the loaded ChannelEntry.
Deduplicated / Rejected Findings
| # | Finding | Severity | Verdict | Reason |
|---|---|---|---|---|
| 2 | Failed Fix & Variants for "Forged Session Hints" | Critical | ⏩ Dup | Same root cause as Finding 1 |
| 4 | Cross-Channel Signature Replay (Confused Deputy) | Critical | ❌ Rejected | Channel-bound signatures prevent cross-service replay |
| 5 | Stale receipt aliasing lets channel A over-authorize channel B | High | ⏩ Dup | Same root cause as Finding 2 |
| 6 | Session hint breaks bodyless control flows on dynamic-priced routes | Medium | ❌ Rejected | Dynamic pricing handled before hint reconciliation |
🔒 Automated security review by Cyclops · 3 engines · 6 raw findings · 2 verified
cd52300 to
8d991bc
Compare
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
Note: Audit was performed against 727181b; current head is 8d991bc. The current head already fixes two issues found during audit (stale channelIdToKey aliases are now deleted in rememberChannel(), and reconcileReceipt() now checks entry.channelId === receipt.channelId). Three findings remain on the current head.
Findings Summary
| # | Tier | Finding | File |
|---|---|---|---|
| 1 | 🚨 SECURITY | Server-supplied requiredCumulative overrides client's locally-tracked authorization |
src/tempo/client/Session.ts:291-293 (pre-existing line, newly reachable via PR) |
| 2 | 🚨 SECURITY | Global fetch polyfill exposes session receipt reconciliation to third-party responses | src/client/internal/Fetch.ts:54, src/tempo/client/Session.ts:496-502 |
| 3 | Unrecoverable server-suggested channelId silently orphans active channel |
src/tempo/client/Session.ts:270 |
Reviewer Callouts
- ⚡
requiredCumulativetrust boundary: Themax(nextCumulative, requiredCumulative)pattern atSession.ts:291-293is the primary remaining trust inversion. The test suite explicitly encodes this as intended ("prefers requiredCumulative"), so this is a design-level decision needing human review. - ⚡ Global fetch blast radius: The combination of
Mppx.create()default polyfill +Fetch.from()response hooks +tempo()bundling session is a cross-cutting concern that only becomes dangerous when all three are active together. - ⚡
reconcileChannelEntryadvisory semantics:acceptedCumulative,spent, anddepositare updated from server hints, butcumulativeAmountis kept local. This separation is correct — except thatrequiredCumulativebypasses it by directly entering the signing path. - ⚡ Channel rebinding policy:
Session.ts:244-254now lets the server drive channel replacement for existing slots. A human reviewer should verify whether any server hint should ever override a still-usable local channel.
7f61f06 to
2065260
Compare
👁️ Cyclops Security Review🧭 Auditing · mode=
Findings
⚙️ Controls
📜 29 events🔍 |
Summary
Implements stateless resume for
tempo/session.A client can reuse an existing Tempo payment channel without persisting local session runtime state, as long as either:
channelId, orThe implementation adds server-side reusable-channel discovery, client-side session hydration from challenge hints, zero-dollar proof bootstrap in
SessionManager, and receipt reconciliation that keeps server accounting authoritative without letting server hints inflate the next signed voucher.What This PR Implements
tempo.session({ resolveSource })so an application can supply the authenticated payer identity from its own auth/session layerrequest.methodDetails:channelId,acceptedCumulative,requiredCumulative,spent, anddepositchannelIdis knownSessionManagerso a stateless client can authenticate first, receive a session challenge, and then resume or open a channelPayment-Receiptand SSE reconciliation that updates local accounting state while preserving the client’s own signing boundarychannelId, which remains the direct reuse path when the client already has itEnd-to-End Flow
sequenceDiagram participant Client participant Server participant Auth as Zero-dollar proof auth participant Session as Session challenge participant Store as Channel store participant Chain as On-chain channel Client->>Server: GET /resource alt auth required and no local session runtime Server-->>Client: 402 + tempo.charge(amount=0) Client->>Auth: sign proof credential Client->>Server: retry with proof credential end Server->>Store: resolve payer and discover reusable channel Server-->>Client: 402 + tempo.session challenge alt explicit or discovered reusable channel available Server-->>Client: channelId + accepted/spent/deposit/required hints Client->>Session: hydrate local runtime state from hints else channelId known but no server snapshot Client->>Chain: recover channel state on-chain Chain-->>Client: settled amount + deposit else no reusable channel Client->>Chain: open new channel end Client->>Server: retry with open/voucher credential Server-->>Client: 200 + Payment-Receipt(channelId, acceptedCumulative, spent) Client->>Session: reconcile receipt into local accounting stateServer Behavior
When the server receives a
tempo/sessionrequest, it can now attach reusable-channel hints to the session challenge.If
request.channelIdis present, that channel is treated as the requested reuse target. The server verifies that the stored channel matches the request dimensions and, when valid, returns challenge hints for that channel.If
request.channelIdis absent andresolveSource()returns a payer DID such asdid:pkh:eip155:<chainId>:<address>, the server:The backing
ChannelStoregains payer-indexed lookup to support this discovery efficiently.Client Behavior
The client now distinguishes between:
acceptedCumulative,spent,depositcumulativeAmountThis distinction is the core of the reconciliation model.
Server hints and receipts can raise the client’s view of accepted/spent/deposit, but they do not directly increase the next signed voucher amount. The next voucher is still derived from the client’s locally authorized cumulative amount plus the current request or stream increment.
This allows the client to resume from server knowledge, process receipts, and handle SSE top-ups without trusting the server to choose a larger authorization boundary than the client intended to sign.
SessionManagernow owns a bounded retry loop for this flow:API and Plumbing Changes
tempo.session()server request hooks now receive the incoming HTTPRequestsoresolveSource()can inspect application auth contextFetch.from()invokes method response hooks for successful retry responses so session receipt reconciliation also works through the generic client pathtempo/sessionrequest parsing accepts additive session hint fields and carries them throughmethodDetailsCompatibility
channelIdreuse continues to work as the direct fast pathresolveSourceis optional; when absent, the flow still supports explicitchannelIdreuse and opening new channels